Skip to content

AuthenticationSupplier feature for Web Security #6496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

sbespalov
Copy link
Contributor

@sbespalov sbespalov commented Feb 1, 2019

This PR targets to make Authentication Filters implementation to be more "SOLID".
Currently there are set of different Atuhenticaion Filters like BasicAuthenticationFilter, DigestAuthenticationFilter etc. and most of such filters logic is almost the same:

  1. expose Authentication from request (supply);
  2. proceed filter chain, if there is no supported Authentication provided
  3. authenticate the Authentication object with AuthenticationManager, if we have supported Authentication

This PR contains following main changes:

  1. Added AuthenticationSupplier interface;
  2. Added GenericAuthenticationFilter class;
  3. Basic Authentication support implemented with BasicAuthenticationSupplier and GenericAuthenticationFilter;

If this concept will be accepted in general then additional changes can be provided.

@pivotal-issuemaster
Copy link

@sbespalov Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@sbespalov Thank you for signing the Contributor License Agreement!

@rwinch
Copy link
Member

rwinch commented Feb 4, 2019

Thanks for the PR @sbespalov!

In general, there are too many changes in this one PR. For example, there are polish actions that add ServletException where it is not necessary.

In regards to the Supplier, I think it may be a good idea, but let's not combine AuthenticationSupplier and AuthenticationEntryPoint. If you look at the reactive bits it uses a ServerAuthenticationConverter which is closer to what I would have in mind.

I'm going to close this PR. If you are still wanting to see this support, please create a ticket for us to discuss a bit so we can be on the same page before writing more code. This will ensure you don't spend a lot of time coding to find out that we are not yet on the same page.

@rwinch rwinch closed this Feb 4, 2019
@rwinch rwinch self-assigned this Feb 4, 2019
@sbespalov
Copy link
Contributor Author

sbespalov commented Feb 5, 2019

@rwinch thanks for your response.

#6506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants